Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bucket: allow to allocate key on stack in Put() #550

Merged
merged 1 commit into from
Aug 11, 2023
Merged

bucket: allow to allocate key on stack in Put() #550

merged 1 commit into from
Aug 11, 2023

Conversation

fyfyrchik
Copy link
Contributor

While doing some bbolt optimizations for my usecase I have noticed key for Bucket.Get() and Bucket.Delete() can be allocated on stack, but when I add Bucket.Put() it cannot. While in reality it could, it seems to be hard for the go compiler.

go version go1.21.0 linux/amd64

As per go build -gcflags -m ./...:

Old behaviour:

./bucket.go:148:31: leaking param: key
./bucket.go:192:42: leaking param: key
./bucket.go:271:22: leaking param: key

Now:

./bucket.go:148:31: key does not escape
./bucket.go:192:42: key does not escape
./bucket.go:271:22: key does not escape

I have performed smoke tests on the benchmark from here #532, and got the expected results: 1 less allocation.

$ benchstat old new
seed: 42
goos: linux
goarch: amd64
pkg: go.etcd.io/bbolt
cpu: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz
                                 │     old     │              new              │
                                 │   sec/op    │   sec/op     vs base          │
Bucket_CreateBucketIfNotExists-8   4.820µ ± 1%   4.825µ ± 2%  ~ (p=1.000 n=10)

                                 │     old      │                 new                 │
                                 │     B/op     │     B/op      vs base               │
Bucket_CreateBucketIfNotExists-8   5.418Ki ± 0%   5.402Ki ± 0%  -0.29% (p=0.000 n=10)

                                 │    old     │                new                │
                                 │ allocs/op  │ allocs/op   vs base               │
Bucket_CreateBucketIfNotExists-8   32.00 ± 0%   31.00 ± 0%  -3.12% (p=0.000 n=10)

@cenkalti
Copy link
Member

cenkalti commented Aug 9, 2023

Cool! Can we have a short comment in code that explains why newKey var is introduced instead of reusing key?

@fyfyrchik
Copy link
Contributor Author

fyfyrchik commented Aug 10, 2023 •
edited
Loading

Fixed, please, take a look.
BTW, #532 is also supposedly affected -- we might want to fix it there or here depending on the PR merge order.

ahrtr
ahrtr reviewed Aug 10, 2023
View reviewed changes
bucket.go Outdated Show resolved Hide resolved
@fyfyrchik
Copy link
Contributor Author

I dived a bit deeper and it seems fixing this in compiler is not that easy (it is not run on SSA form yet, so all key are equal):
golang/go#18300

@ahrtr
Copy link
Member

ahrtr commented Aug 10, 2023

BTW, #532 is also supposedly affected -- we might want to fix it there or here depending on the PR merge order.

The PR #532 has already been merged. Please rebase this PR and fix it as well. thx

@fyfyrchik
bucket: allow to allocate key on stack in Put()
71a59ca 
As per `go build -gcflags -m ./... 2>&1`:

Old behaviour:
```
./bucket.go:148:31: leaking param: key
./bucket.go:192:42: leaking param: key
./bucket.go:271:22: leaking param: key
```

Now:
```
./bucket.go:148:31: key does not escape
./bucket.go:192:42: key does not escape
./bucket.go:271:22: key does not escape
```

Signed-off-by: Evgenii Stratonikov <fyfyrchik@runbox.com>
@fyfyrchik fyfyrchik requested a review from ahrtr August 10, 2023 12:28
ahrtr
ahrtr approved these changes Aug 10, 2023
View reviewed changes
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thank you! @fyfyrchik

@ahrtr ahrtr merged commit 59b8453 into etcd-io:master Aug 11, 2023
@fyfyrchik fyfyrchik deleted the heap-alloc-key branch August 11, 2023 09:55
@fuweid fuweid mentioned this pull request Dec 13, 2023
Merged
fuweid added a commit to fuweid/bbolt that referenced this pull request Dec 18, 2023
@fuweid
CHANGELOG: update for v1.3.9
80a557f 
- [bucket: allow to allocate key on stack in Put()](etcd-io#550)
- [bucket.Put: copy key before seek](etcd-io#637)
- [copy key before comparing during CreateBucket or CreateBucketIfNotExists](etcd-io#641)

Signed-off-by: Wei Fu <fuweid89@gmail.com>
@fuweid fuweid mentioned this pull request Dec 18, 2023
Merged
fuweid added a commit to fuweid/bbolt that referenced this pull request Dec 18, 2023
@fuweid
CHANGELOG: update for v1.3.9
e4ab08e 
- [bucket: allow to allocate key on stack in Put()](etcd-io#550)
- [bucket.Put: copy key before seek](etcd-io#637)
- [copy key before comparing during CreateBucket or CreateBucketIfNotExists](etcd-io#641)

Signed-off-by: Wei Fu <fuweid89@gmail.com>
ishan16696 pushed a commit to ishan16696/bbolt that referenced this pull request Feb 25, 2024
@fuweid @ishan16696
CHANGELOG: update for v1.3.9
d8bf461 
- [bucket: allow to allocate key on stack in Put()](etcd-io#550)
- [bucket.Put: copy key before seek](etcd-io#637)
- [copy key before comparing during CreateBucket or CreateBucketIfNotExists](etcd-io#641)

Signed-off-by: Wei Fu <fuweid89@gmail.com>
passiondev2024 added a commit to passiondev2024/bbolt that referenced this pull request Mar 20, 2024
@passiondev2024
CHANGELOG: update for v1.3.9
659c0ce 
- [bucket: allow to allocate key on stack in Put()](etcd-io/bbolt#550)
- [bucket.Put: copy key before seek](etcd-io/bbolt#637)
- [copy key before comparing during CreateBucket or CreateBucketIfNotExists](etcd-io/bbolt#641)

Signed-off-by: Wei Fu <fuweid89@gmail.com>
samuelbartels20 pushed a commit to samuelbartels20/bbolt that referenced this pull request Nov 10, 2024
@fuweid @samuelbartels20
CHANGELOG: update for v1.3.9
4c03d3a 
- [bucket: allow to allocate key on stack in Put()](etcd-io#550)
- [bucket.Put: copy key before seek](etcd-io#637)
- [copy key before comparing during CreateBucket or CreateBucketIfNotExists](etcd-io#641)

Signed-off-by: Wei Fu <fuweid89@gmail.com>
Signed-off-by: samuelbartels20 <bartelssamuel20@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahrtr ahrtr ahrtr approved these changes

@serathius serathius serathius approved these changes

Assignees
No one assigned
Labels
area/performance
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fyfyrchik @cenkalti @ahrtr @serathius